You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

Self-Review

Correctness

• Domain: Dismiss() correctly guards against non-terminal states (PendingReview, Approved). Only Applied, Rejected, Failed, Expired can be dismissed.
• Service: DismissProposalsAsync silently skips proposals that can't be dismissed (e.g. if status changed between the controller check and the service call). This is intentional -- partial success is better than failing the whole batch.
• Controller: Authorization checks ownership via GetProposalsAsync with the caller's userId. The unauthorizedIds check prevents dismissing other users' proposals.
• Frontend: visibleProposals always hides Dismissed, and hides other terminal states when toggle is off. Summary cards react to filtered list.

Edge cases considered

1. Ownership query bounded at 500: If a user has >500 proposals, old ones won't appear in the ownership check, potentially blocking dismiss of their own proposals. Low risk -- 500 is a generous limit for active proposals.
2. Race condition in frontend dismiss: IDs captured before API call may diverge from local state after. Mitigated by Set-based removal and the filter already hiding terminal proposals.
3. Applied summary card shows 0 when toggle off: Expected behavior per spec -- badges update with filter.

Security

• Endpoint requires [Authorize] via controller class attribute.
• Ownership check prevents cross-user dismissal.
• Batch size capped at MaxProposalListLimit (500) to prevent abuse.
• No board-level write access check for dismiss, but dismiss is a view-layer operation (low risk).

Style

• Follows existing C# conventions (PascalCase, 4-space indent).
• Frontend uses composition API, <script setup>, Tailwind design tokens.
• Commits are focused and small.

No issues requiring fixes found.

Adversarial Code Review: PR #634 -- UX-17 Dismiss Proposals

Critical

1. GetByIdsAsync uses AsNoTracking -- dismissed proposals will never be persisted

DismissProposalsAsync in AutomationProposalService calls _unitOfWork.AutomationProposals.GetByIdsAsync(ids, cancellationToken) and then mutates the returned entities with proposal.Dismiss(). However, GetByIdsAsync in AutomationProposalRepository (line 70) uses .AsNoTracking():

return await _dbSet
    .AsNoTracking()
    .Where(proposal => uniqueIds.Contains(proposal.Id))
    .ToListAsync(cancellationToken);

EF Core will not track these entities, so when SaveChangesAsync is called, no UPDATE statements are emitted. The endpoint will report success but the status change is silently lost. The next refresh will show the proposals again, undismissed.

Fix: Either (a) add a tracked overload (e.g., GetByIdsForUpdateAsync) that omits AsNoTracking, or (b) fetch via GetByIdAsync per entity (which already tracks), or (c) call _dbSet.AttachRange(...) / DbContext.Update(...) before saving.

High

2. Authorization check has a hard ceiling at 500 -- silent false-Forbidden for prolific users

In the controller's DismissProposals action:

var proposals = await _proposalService.GetProposalsAsync(
    new ProposalFilterDto(null, null, callerUserId, null, MaxProposalListLimit),
    cancellationToken);

var ownedIds = proposals.Value.Select(p => p.Id).ToHashSet();
var unauthorizedIds = request.Ids.Where(id => !ownedIds.Contains(id)).ToList();

MaxProposalListLimit is 500. If a user has >500 proposals, older proposals will not appear in ownedIds. The user will receive a 403 Forbidden when attempting to dismiss their own valid proposal, simply because it fell outside the top-500 window.

Fix: Do not use GetProposalsAsync for authorization. Instead, fetch the specific proposals by ID (via GetByIdsAsync or GetByIdAsync), and verify RequestedByUserId == callerUserId on each entity. This is both correct and more efficient than loading 500 proposals to check a subset.

3. New enum value Dismissed is not in ReviewedStatuses -- future queries may silently exclude dismissed proposals

AutomationProposalRepository.ReviewedStatuses (line 11-17) lists Approved, Rejected, Applied, Failed -- but not Dismissed. This means HasReviewedByUserIdAsync treats dismissed proposals correctly (they were previously in a reviewed state, so the original status was already counted). However, any future code that uses ReviewedStatuses as "all terminal states" will silently exclude Dismissed.

Fix: Either add Dismissed to ReviewedStatuses, or rename the array to make its semantic clear (e.g., DecisionOutcomeStatuses) so future developers do not assume it means "all non-pending states."

Medium

4. Broad DomainException catch in DismissProposalsAsync hides bugs

foreach (var proposal in proposals)
{
    try
    {
        proposal.Dismiss();
        dismissed++;
    }
    catch (DomainException)
    {
        // Skip proposals that cannot be dismissed (e.g. still Pending/Approved)
    }
}

The inner catch swallows all DomainException -- including unexpected ones from Touch() or future domain logic. Since Dismiss() already has an explicit status guard, the caller should pre-filter to dismissable statuses instead of relying on exception flow for normal control.

Fix:

foreach (var proposal in proposals)
{
    if (proposal.Status is not (ProposalStatus.Applied or ProposalStatus.Rejected
        or ProposalStatus.Failed or ProposalStatus.Expired))
        continue;

    proposal.Dismiss();
    dismissed++;
}

This also avoids using exceptions for expected control flow, which is a performance concern in hot paths.

5. Dismissed is in completedStatuses AND has an explicit hide-always check -- redundant logic

In ReviewView.vue:

const completedStatuses = new Set(['Applied', 'Rejected', 'Failed', 'Expired', 'Dismissed'])

// ...
if (normalizeProposalStatus(proposal.status) === 'Dismissed') return false
if (!showCompleted.value && completedStatuses.has(normalizeProposalStatus(proposal.status))) return false

The first check is redundant because the second check already hides Dismissed when showCompleted is off. The only case the first check adds is hiding Dismissed when showCompleted is ON. If that is the intent, it should be documented with a comment. If not, remove the redundant check and take Dismissed out of completedStatuses.

Fix: If the intent is "dismissed are always hidden even when Show Completed is on," then remove Dismissed from completedStatuses (it is dead code there) and keep only the explicit check with a comment:

// Dismissed proposals are always hidden -- they are permanently cleared
if (normalizeProposalStatus(proposal.status) === 'Dismissed') return false

const completedStatuses = new Set(['Applied', 'Rejected', 'Failed', 'Expired'])
if (!showCompleted.value && completedStatuses.has(normalizeProposalStatus(proposal.status))) return false

6. No EF migration for the new enum value

ProposalStatus.Dismissed (value 6) is added to the C# enum. If EF Core stores this as an integer column (the default for SQLite), existing rows are fine -- but there is no migration to validate the schema snapshot is up to date. If the column uses string conversion, existing queries filtering by status will need to handle the new value.

Fix: Generate an EF migration (dotnet ef migrations add AddDismissedProposalStatus) even if it is empty, to keep the model snapshot in sync. Verify the storage format (int vs. string) and confirm the migration path is clean.

Low

7. Button label says "Clear applied" but action dismisses all terminal states

The button text reads "Clear applied (N)" but it dismisses Applied, Rejected, Failed, AND Expired proposals. This is misleading UX copy.

Fix: Rename to "Clear completed" to match the actual behavior.

8. Frontend removes dismissed proposals from local state optimistically

const dismissedSet = new Set(ids)
proposals.value = proposals.value.filter((p) => !dismissedSet.has(p.id))

This removes ALL requested IDs from the local list regardless of whether the backend actually dismissed them (some may have been skipped due to status). The result.dismissed count could be less than ids.length, but the UI removes all of them.

Fix: Either (a) have the backend return the list of actually-dismissed IDs so the frontend can filter accurately, or (b) after dismiss, re-fetch the proposal list from the server.

9. No backend tests for the new Dismiss() domain method or DismissProposalsAsync service method

The PR mentions "Backend tests pass (1572/1572)" but there are no new test cases for:

• Dismiss() succeeding from each valid source state (Applied, Rejected, Failed, Expired)
• Dismiss() throwing from invalid states (PendingReview, Approved, already Dismissed)
• DismissProposalsAsync partial-success scenarios
• The controller endpoint authorization and edge cases

Given the project's testing guidelines ("Behavior changes ship with tests"), this is a gap.

Verdict

Finding #1 (Critical) is blocking. The dismiss operation silently does nothing due to AsNoTracking. This must be fixed before merge.

Finding #2 (High) is blocking. The authorization logic has a concrete failure mode for users with >500 proposals. The fix is straightforward -- authorize against the fetched proposals, not a separate capped query.

The remaining findings are non-blocking but should be addressed -- especially #4 (exception swallowing) and #9 (missing tests).

Adversarial review fixes (3d2376a)

Addressed all blocking issues from the adversarial review:

1. CRITICAL: GetByIdsAsync returned untracked entities — Dismiss() silently did nothing

Root cause: AutomationProposalRepository.GetByIdsAsync used .AsNoTracking(), so Dismiss() mutated in-memory status but EF Core emitted no UPDATE on SaveChangesAsync.
Fix: Removed AsNoTracking() from GetByIdsAsync so returned entities are change-tracked and mutations persist.

2. HIGH: Authorization capped at 500 proposals

Root cause: Controller pre-fetched all user proposals (capped at MaxProposalListLimit = 500) to build an ownership set, causing false 403s for users with >500 proposals.
Fix: Replaced with per-ID ownership check — fetches only the specific proposals being dismissed and verifies RequestedByUserId on each.

3. NON-BLOCKING: Broad DomainException catch

Root cause: DismissProposalsAsync used a try/catch on DomainException as flow control to skip non-dismissible proposals.
Fix: Replaced with explicit status guard (if proposal.Status is Applied or Rejected or Failed or Expired) before calling Dismiss().

All 1929 backend tests pass.